Rust: make mcap-rs wasm compatible #989
Conversation
|
|
|
Thanks for opening this effort and the writeup; this is definitely a feature that I think makes the rust library more desirable and we should add this capability. Sharing some of my thoughts:
Is there a feature flag for zstd? If not - why does only lz4 get a feature flag?
If the new library supports writing and reading lz4 in a way that the existing library also supports then I don't see a problem. I don't know if anything specific went into picking
Do we need to turn off the default features? It isn't clear to me if turning off the default is a requirement to turning wasm on. cc @mrkline |
Yes, and it is already on main. Line 30 in c687014
I generated several mcap files and tried without a problem. Let me know if we should add a test for this.
Good point. I copied the code from another post and did not verify by myself. Just tried again and confirmed we don't need to turn off the default features. Pushed a commit to fix this. |
There was a problem hiding this comment.
Nice!
As a nit I'd say that it would be nicer to have cleanup of unrelated code (e.g., reformatting, self: &Self -> &self, etc.) as a seperate PR, but wider support for compression is great in my book!
FWIW, I don't think there was any deep reason for chosing lz4 other than that it's a popular crate. 😅
rust/Cargo.toml
Outdated
| keywords = ["foxglove", "mcap"] | ||
| categories = ["science::robotics", "compression"] |
There was a problem hiding this comment.
Was this some auto-formatter?
There was a problem hiding this comment.
Yes. I can revert it.
| [target.'cfg(target_arch = "wasm32")'.dependencies] | ||
| zstd = { version = "0.11", features = ["wasm"], optional = true } | ||
|
|
||
| [target.'cfg(not(target_arch = "wasm32"))'.dependencies] |
There was a problem hiding this comment.
Nice - appreciate you only pulling in wasm support when it's the target!
|
Pushed two commits to revert all the irrelevant changes made by the linter and formatter. Let me know if this looks good to you. |
### Public-Facing Changes * Dependency is changed from [lz4](https://github.com/10xGenomics/lz4-rs) to [lz4_flex](https://github.com/PSeitz/lz4_flex) * A new feature flag `lz4` is introduced and enabled by default ### Description This change makes mcap-rs wasm compatible. * Replace [lz4](https://github.com/10xGenomics/lz4-rs) with [lz4_flex](https://github.com/PSeitz/lz4_flex), because the former one depends on `lz4-sys`, which is not easy to port to wasm. * Add a feature flag `lz4` so the library user can choose to opt-out of the dependency. * ~~Turn off the default features and~~ turn on the `wasm` feature for [zstd](https://github.com/gyscos/zstd-rs) when the target platform is wasm. * Add Clippy checking in the CI for all combinations of features. * Minor fixes for Clippy linter warnings.
Public-Facing Changes
lz4is introduced and enabled by defaultDescription
I am trying to use
mcap-rsin an application that targets multiple platforms, including native and wasm. I modified the source code to make it wasm compatible, and think this may be also useful for others.The changes include:
lz4-sys, which is not easy to port to wasm.lz4so the library user can choose to opt-out of the dependency.Turn off the default features andturn on thewasmfeature for zstd when the target platform is wasm.I am not quite sure if it would be a good idea to change the
lz4library. It is also possible to still use lz4 library and opt-out of it when the platform is wasm.I haven't raised an issue yet, because I am not sure if this belongs to a bug fix or feature request. (Feature request always leads me to the "discussion" page. Is that the correct place?) Please let me know where I should create it.